Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Support password authentication #88

Merged
merged 11 commits into from
Apr 21, 2020
Merged

Support password authentication #88

merged 11 commits into from
Apr 21, 2020

Conversation

nfnt
Copy link
Contributor

@nfnt nfnt commented Apr 16, 2020

Calls by the operator to nodetool are authenticated with credentials provided as a secret by the admin.

@nfnt nfnt added the wip label Apr 16, 2020
@nfnt nfnt self-assigned this Apr 16, 2020
@nfnt nfnt force-pushed the nfnt/authn branch 5 times, most recently from 82281f2 to b3752d8 Compare April 17, 2020 09:56
@nfnt nfnt marked this pull request as ready for review April 17, 2020 14:01
operator/templates/node-scripts.yaml Outdated Show resolved Hide resolved
operator/templates/repair-job.yaml Outdated Show resolved Hide resolved
operator/templates/stateful-set.yaml Outdated Show resolved Hide resolved
templates/operator/params.yaml.template Outdated Show resolved Hide resolved
Copy link
Contributor

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks fine... I agree with @ANeumann82 points since he has a reject on it I'll assume it's all good after that is resolved... I'm good without the changes as well... the changes seems useful but not critical

operator/templates/node-scripts.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,111 @@
package authentication

import (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like it could be a kuttl test which might be easier to read and consume

@nfnt nfnt added ready for review and removed wip labels Apr 20, 2020
@nfnt nfnt requested a review from ANeumann82 April 20, 2020 08:58
Copy link
Contributor

@zmalik zmalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking great!

I have two suggestions:

  • not using env variables to populate the password. If we can use a filebased approach that would be slightly better.
  • add authentication credentials also to cqlshrc file, otherwise cqlsh queries will fail.

cqlshrc can be part of another PR but I feel we should avoid passwords living in env variables. WDYT?

operator/templates/node-scripts.yaml Show resolved Hide resolved
@nfnt
Copy link
Contributor Author

nfnt commented Apr 20, 2020

Passwords in env variables are fine as long as they're not exported. But you raise a good point here, because passwords as part of a command line are not fine because they can be extracted by a ps call. Just noticed that TLS_KEYSTORE_PASSWORD and TLS_TRUSTSTORE_PASSWORD are used like this and with cleartext password in parameters as well. I'll take a look on how to secure that. nodetool supports password files, let me see how this can be added.

Copy link
Contributor

@zmalik zmalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@nfnt
Copy link
Contributor Author

nfnt commented Apr 21, 2020

There's no good way to fully secure nodetool authentication in a cluster environment. Or at least in the way we use that tool 😄. Especially the kubectl exec -- nodetool of the repair job is problematic. We should think of a better way to provide this functionality; a sidecar with some API endpoints could improve the situation here.

@nfnt nfnt merged commit 76583a0 into master Apr 21, 2020
@nfnt nfnt deleted the nfnt/authn branch April 21, 2020 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants